Skip to content

type(feat): Implemented PanelUI Classes and Tests#338

Merged
b4handjr merged 10 commits intomozilla:mainfrom
Temidayo32:panelui
Feb 26, 2025
Merged

type(feat): Implemented PanelUI Classes and Tests#338
b4handjr merged 10 commits intomozilla:mainfrom
Temidayo32:panelui

Conversation

@Temidayo32
Copy link
Copy Markdown
Collaborator

@Temidayo32 Temidayo32 commented Jan 26, 2025

Because

  • This commit implements PanelUI functionality

This commit

  • Implemented PanelUI Class and Tests

Fixes #313

@Temidayo32
Copy link
Copy Markdown
Collaborator Author

Temidayo32 commented Jan 26, 2025

This is still WIP. Failure is because tests do not meet 95% coverage yet.

@Temidayo32 Temidayo32 changed the title Implemented PanelUI Classes and Tests type(feat): Implemented PanelUI Classes and Tests Feb 3, 2025
@Temidayo32 Temidayo32 requested a review from b4handjr February 3, 2025 22:05
@Temidayo32 Temidayo32 force-pushed the panelui branch 4 times, most recently from 731b778 to 3d6cc7a Compare February 4, 2025 18:19
@Temidayo32 Temidayo32 force-pushed the panelui branch 5 times, most recently from e548f64 to 8d8f563 Compare February 5, 2025 14:26
Comment on lines +173 to +175
def verify_links_in_awesome_bar(self, links: list, new_window: bool = False) -> list:
"""
Verifies that the provided links appear in the awesome bar.
Args:
links `list`: A list of links to be verified.

Returns:
list: A list of links that were not found in the awesome bar.
"""
if new_window:
self.open_new_window()
else:
self.open_new_tab()
self.switch_to_new_window_or_tab()
for link in links:
self.selenium.get(link)
return self.awesome_bar(links)

def verify_private_browsing_links_not_in_awesome_bar(self, links: list) -> list:
"""
Verifies that the provided links visited in private browing session do not appear in the awesome bar.
Args:
links `list`: A list of links to be verified.

Returns:
list: A list of links that appeared in the awesome bar during private browsing.
"""
initial_window_handle = self.selenium.current_window_handle
self.open_private_window()
self.switch_to_new_window_or_tab()

for link in links:
self.selenium.get(link)

self.selenium.switch_to.window(initial_window_handle)
return self.awesome_bar(links)

def verify_links_in_history(self, links: list, new_window: bool = False) -> list:
"""
Verifies that the provided links appear in the history.
Args:
links `list`: A list of links to be verified.

Returns:
list: A list of links that were not found in the history.
"""
if new_window:
self.open_new_window()
else:
self.open_new_tab()
self.switch_to_new_window_or_tab()
for link in links:
self.selenium.get(link)
self.open_panel_menu()
self.open_history_menu()
urls = []
for link in links:
if not self.is_present(link):
urls.append(link)
return urls

def verify_private_browsing_links_not_in_history(
self,
links: list,
) -> list:
"""
Verifies that the provided links visited in private browsing session do not appear in the history.
Args:
links (`list[str]`): List of URLs visited during private browsing session
history (`History`): An instance of the History class used to check if a link is present in the history.

Returns:
list: A list of links that were found in the history during the private browsing session.
"""
invalid_links = []
initial_window_handle = self.selenium.current_window_handle
self.open_private_window()
self.switch_to_new_window_or_tab()

for link in links:
self.selenium.get(link)

self.selenium.switch_to.window(initial_window_handle)
self.open_panel_menu()
self.open_history_menu()
for link in links:
if self.is_present(link):
invalid_links.append(link)
return invalid_links

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the logic behind these additions? These seem like actions that a user would do. Are you expecting a user to supply a list of URLs for these methods to verify? Why wouldn't they just do that themselves?

Copy link
Copy Markdown
Collaborator Author

@Temidayo32 Temidayo32 Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right. The actions mirror what a user would do manually, but given that this is a test automation tool, I think its purpose is always to automate manual user actions. Because technically, this isn't user functionality - it's testing infrastructure. In this sense, I try to automate certain expected behaviors of Private Window browsing vs Normal Window browsing, history etc.

Copy link
Copy Markdown
Collaborator Author

@Temidayo32 Temidayo32 Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the user will provide a list of URLs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember, we are just trying to programmatically represent a way for a user to interact with Firefox. We don't want to assume what combinations they will do, just provide them a way to do them.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that makes good sense. I will move those methods to tests. So they serve as part of FoxPuppet test suite. Will that be good or I just remove them totally.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heading in the right direction with this. I think there will need to be some sort of History class so that we can contain the History specific interactions in one place.

)
return self.bookmark

def wait_for_panel_ui(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would a user want to wait for the panel ui to show? Can you explain a scenario?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual correct docstring:
Wait for the specified PanelUI item to be displayed.
That was misleading. There is no reason to wait for panel_ui as it is visible once the browser is opened.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PanelUI item, here, referring to menu items in the Panel Ui dropdown. History, for instance.

selenium.get(url)
panel_ui.open_history_menu()
panel_ui.clear_history()
import time
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can put this import at the top even if you just use it here

@Temidayo32
Copy link
Copy Markdown
Collaborator Author

Heading in the right direction with this. I think there will need to be some sort of History class so that we can contain the History specific interactions in one place.

Yes, I tried to do that initially. But this was difficult using the same property(history) and wait_for_history method defined on the BrowserWindow class as we did with others, especially using the Panel-UI, which in the dom is a list and all items are visible. Suppose we decided to extend later on by adding more list items like PrivateWindow or Download. In that case, it will prove difficult to interact with a specific one since they will all be visible.

Copy link
Copy Markdown
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent progress. I see some tests here that aren't quite testing the functionality of FoxPuppet but more so the functionality of Firefox. Can you identify which tests those are?

Comment on lines +44 to +65
"""Check if the provided links are present in the url bar's suggestions."""
urls = []
with self.selenium.context(self.selenium.CONTEXT_CHROME):
for link in links:
url_bar = self.selenium.find_element(*NavBarLocators.INPUT_FIELD)
url_bar.clear()
url_bar.send_keys(link)

self.wait.until(
lambda _: self.selenium.find_elements(*NavBarLocators.SEARCH_RESULTS)
)

search_results = self.selenium.find_elements(
*NavBarLocators.SEARCH_RESULT_ITEMS
)

for result in search_results:
url_span = result.find_element(*NavBarLocators.SEARCH_RESULT_ITEM)
if url_span.text in link:
if len(url_span.text) != 0:
urls.append(link)
break
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a separate method. If you need to create a UrlBar class to house it, that is fine, but this url_bar should be a property that just returns a class.

return panel_items.get(_id, PanelUI)(window, root)

@property
def is_barged(self) -> bool:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see where you get this name from but would a user understand what barged means? Honestly, if I didn't see the get_attribute("barged") I wouldn't know what it meant.

@Temidayo32 Temidayo32 requested a review from b4handjr February 12, 2025 17:12
Copy link
Copy Markdown
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there. I think we can remove some checks and functions but there are also a few methods that we need to make sure they're doing what they say they will do.

Comment on lines +16 to +38
def check_suggestions(self, links: list) -> list:
"""Check if the provided links are present in the URL bar's suggestions."""
urls = []
with self.selenium.context(self.selenium.CONTEXT_CHROME):
for link in links:
url_bar = self.selenium.find_element(*URLBarLocators.INPUT_FIELD)
url_bar.clear()
url_bar.send_keys(link)

self.wait.until(
lambda _: self.selenium.find_elements(*URLBarLocators.SEARCH_RESULTS)
)

search_results = self.selenium.find_elements(
*URLBarLocators.SEARCH_RESULT_ITEMS
)

for result in search_results:
url_span = result.find_element(*URLBarLocators.SEARCH_RESULT_ITEM)
if url_span.text in link and len(url_span.text) != 0:
urls.append(link)
break
return urls
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's save this for now and just return the URLs that get shown in the url bar that are suggested.

Copy link
Copy Markdown
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I think we're almost there.

Copy link
Copy Markdown
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice changes! Just a couple things and ill have Ben C look at this too.

@Temidayo32 Temidayo32 force-pushed the panelui branch 3 times, most recently from 4e18ec3 to 19a0b38 Compare February 21, 2025 13:44
Comment on lines +2 to +3
addopts = -vvv --driver Firefox --cov --cov-fail-under=95 --html=results/report.html --self-contained-html
testpaths = tests No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a newline

Copy link
Copy Markdown
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding these based on our chat

""",
self.selenium.find_element(*PanelUILocators.HISTORY_DIALOG_BUTTON),
)
self.selenium.switch_to.default_content()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need to switch_to since we are using with

Comment on lines +152 to +155
def clear_history(self):
"""
Clears the browsing history.
"""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can confirm the history is cleared some way. What do you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the history_items method

Copy link
Copy Markdown
Collaborator

@ben-c-at-moz ben-c-at-moz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good, I think addressing that sleep command is the only new thing I can find in here that I think needs to change. I would definitely make absolutely sure you don't run find_elements() on anything until you KNOW it exists, as it doesn't include the same implicit wait as find_element() does.

Comment on lines +98 to +102
self.wait.until(
lambda _: self.selenium.execute_script("return document.readyState")
== "complete",
message="New window document not fully loaded",
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this more accurately addresses the root issue compared to time.sleep

Copy link
Copy Markdown
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

! final thing and I think this is good

selenium.get(url)
panel_ui.open_history_menu()
browser_history.clear_history()
time.sleep(1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still need this sleep?

Copy link
Copy Markdown
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!! Thanks for working on this and sticking with it, this is a great addition and will go a long way to helping us expand FoxPuppets offerings!

@b4handjr b4handjr merged commit 6783fd2 into mozilla:main Feb 26, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add PanelUI functionality

3 participants